Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set the charset/collation of the table/column on base of the connection charset/collation #192

Closed
wants to merge 5 commits into from

Conversation

Jako
Copy link
Collaborator

@Jako Jako commented Mar 8, 2021

Fix #175

A port to 3.x is possible. I will create it if it is accepted here – it was easier for me to test it in 2.x.

@cla-bot cla-bot bot added the cla-signed CLA confirmed for all contributors to this PR label Mar 8, 2021
@netProphET
Copy link
Member

👑

@Jako Jako changed the title Set the charset of the table/column on base of the connection charset Set the charset/collation of the table/column on base of the connection charset/collation Apr 7, 2021
@Jako
Copy link
Collaborator Author

Jako commented Apr 7, 2021

I have added some code to cover setting the collation too. The collation is currently not defined in the MODX (database) config, so it will use the MySQL default collation of the charset defined by MODX.

@Mark-H
Copy link
Collaborator

Mark-H commented Apr 7, 2021

So if I understand correctly, this adds the support to xPDO to specify a charset and collation into the xPDOConnection options, but it does not yet automatically set them. Basically, keeping the current behavior but opening the door to change how we use it in MODX and/or extras?

@Jako
Copy link
Collaborator Author

Jako commented Apr 9, 2021

It make it possible to create a database table with the connection charset (which is set i.e. in the MODX config) to create a new database table (the connection collation is not set in the MODX config). This solves the big issue of not creating a new database table with the MODX connection charset but with the MySQL connection charset/collation.

@Mark-H
Copy link
Collaborator

Mark-H commented Jan 25, 2022

@opengeek Any thoughts on this? It came up, again, and this fix seems like it ought to do the trick?

@@ -436,6 +445,13 @@ protected function getColumnDef($class, $name, $meta, array $options = array())
if (empty ($extra) && isset ($meta['extra'])) {
$extra= ' ' . $meta['extra'];
}
$charset = '';
if (!empty($options['charset']) && in_array($this->xpdo->driver->getPhpType($dbtype), array('string'))) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to support other PHP types (e.g. array, which gets serialized automatically) here? Perhaps this ought to check the dbtype for a whitelist of column types that support character sets instead?

@Mark-H Mark-H requested a review from opengeek February 1, 2022 09:40
@opengeek
Copy link
Member

opengeek commented Feb 1, 2022

As has been stated multiple times, this is a database creation issue. Nothing else. The only time this is an issue is on platforms where you cannot let MODX create the database. Thus, this is user error. Trying to force this on a per table or column definition basis is not the proper way and reduces the flexibility of how xPDO can be used, IMO. The connection charset can be changed and is transient. It is meant to allow connection-based translations of data from the charset/collation of the database/table/column charset/collation. I would think relying on this for table creation and modification could create just as much of a mess in situations other than MODX installation.

I'm going to step back and look at this again in-depth after RC2 release this week, but my gut still says this is not the way.

@opengeek opengeek self-assigned this Feb 1, 2022
@Jako
Copy link
Collaborator Author

Jako commented Feb 1, 2022

This has been stated multiple times too: During the MODX Setup the user is able to set/change the charset of the database (which is used later on as the connection charset). Selecting a different connection charset than the database charset is not only a user error, because it is possible. The current implementation of the createObjectContainer method does not allow to set ObjectContainer options like the createSourceContainer method. Could the createObjectContainer method be extended by a second property to make using a different charset possible?

@Jako Jako closed this Jan 31, 2023
@Jako Jako deleted the create_charset branch January 31, 2023 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for all contributors to this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants